fix(lsp): improve Windows server detection and avoid unix 'which' in doctor by MotorwaySouth9 · Pull Request #834 · code-yeongyu/oh-my-opencode

MotorwaySouth9 · 2026-01-16T00:43:19Z

Summary

Fix Windows compatibility issues that make LSP features effectively unusable in the oh-my-opencode plugin (not opencode core). The main problems were:

The doctor LSP check relied on unix-only which
LSP server detection did not fully account for Windows executable resolution (npm *.cmd shims, PATHEXT, and Path casing)

Closes #832

Changes

Doctor: remove unix-only `which`

Updated src/cli/doctor/checks/lsp.ts to stop spawning which and instead reuse the internal resolver (isServerInstalled).
Added a regression test in src/cli/doctor/checks/lsp.test.ts to ensure the LSP doctor check does not spawn which.

LSP server detection: Windows-aware executable resolution

Updated src/tools/lsp/config.ts (isServerInstalled) to:
- honor PATHEXT on Windows
- always include safe default extensions (.exe, .cmd, .bat, .ps1) as fallback
- handle Path vs PATH env var casing on Windows

Tests

Added src/tools/lsp/config.test.ts to cover Windows PATHEXT/Path behavior and non-Windows behavior.

Testing

bun test src/cli/doctor/checks/lsp.test.ts
bun test src/tools/lsp/config.test.ts
bun run typecheck

Notes

Full bun test on Windows may currently fail due to unrelated pre-existing platform/path assumptions in other tests. The LSP-related tests listed above pass.

Summary by cubic

Fix Windows LSP detection and remove the Unix-only which call in the doctor, restoring LSP usability on Windows. Closes #832.

Bug Fixes
- Doctor: stop spawning which; reuse isServerInstalled for binary checks.
- Windows detection: respect PATHEXT, add safe defaults (.exe, .cmd, .bat, .ps1), and handle Path vs PATH.
- Tests: add coverage for Windows resolution and ensure doctor doesn’t spawn which.

^{Written for commit 8e02cab. Summary will update on new commits.}

cubic-dev-ai

2 issues found across 4 files

Confidence score: 3/5

Restoring PATH/Path incorrectly in src/tools/lsp/config.test.ts can leave the test environment polluted for subsequent specs, making failures non-deterministic.
The mock cleanup in src/cli/doctor/checks/lsp.test.ts isn’t exception-safe, so a thrown test leaves Bun.spawn patched for later tests, potentially hiding real regressions.
Pay close attention to src/tools/lsp/config.test.ts, src/cli/doctor/checks/lsp.test.ts - environment and mock cleanup can leak across tests.

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/cli/doctor/checks/lsp.test.ts">

<violation number="1" location="src/cli/doctor/checks/lsp.test.ts:23">
P2: Bun.spawn spy is not guaranteed to be restored if the test throws, so the mock can leak into subsequent tests.</violation>
</file>

<file name="src/tools/lsp/config.test.ts">

<violation number="1" location="src/tools/lsp/config.test.ts:27">
P2: Teardown restores PATH/Path incorrectly on Windows; deleting "Path" after setting PATH wipes the restored value due to case-insensitive env vars, leaking env changes across tests.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

src/cli/doctor/checks/lsp.test.ts

src/tools/lsp/config.test.ts

cubic-dev-ai · 2026-01-16T01:03:03Z

Thanks @cubic-dev-ai — good catches.\n\nApplied the suggested fixes:\n- Make spy restore exception-safe via in \n- Fix PATH/Path restore on Windows (case-insensitive env keys) by restoring PATH/Path as a single unit and restoring PATHEXT separately in \n\nLocal verification:\n- bun test v1.3.6 (d530ed99)\n- bun test v1.3.6 (d530ed99)\n- \n\nCould you please re-run the review?

@MotorwaySouth9 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai · 2026-01-16T01:03:15Z

Thanks @cubic-dev-ai — good catches.

Applied the suggested fixes:

Make \�[0m\�[0m spy restore exception-safe via \�[0m\�[0m in \�[0m\�[0m

Fix PATH/Path restore on Windows (case-insensitive env keys) by restoring PATH/Path as a single unit and restoring PATHEXT separately in \�[0m\�[0m
...

@MotorwaySouth9 I have started the AI code review. It will take a few minutes to complete.

MotorwaySouth9 · 2026-01-16T01:03:20Z

Apologies — my previous comment got mangled by shell quoting.

Thanks @cubic-dev-ai — good catches (identified by cubic).

Applied the suggested fixes:

Make Bun.spawn spy restore exception-safe via try/finally in src/cli/doctor/checks/lsp.test.ts
Fix PATH/Path restore on Windows (case-insensitive env keys) by restoring PATH/Path as a single unit and restoring PATHEXT separately in src/tools/lsp/config.test.ts

Local verification:

bun test src/cli/doctor/checks/lsp.test.ts
bun test src/tools/lsp/config.test.ts
bun run typecheck

Could you please re-run the review?

cubic-dev-ai · 2026-01-16T01:03:38Z

Apologies — my previous comment got mangled by shell quoting.

Thanks @cubic-dev-ai — good catches (identified by cubic).

Applied the suggested fixes:
...

@MotorwaySouth9 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai · 2026-01-16T01:04:51Z

Thanks @cubic-dev-ai — good catches.

Applied the suggested fixes:

Make \�[0m\�[0m spy restore exception-safe via \�[0m\�[0m in \�[0m\�[0m

Fix PATH/Path restore on Windows (case-insensitive env keys) by restoring PATH/Path as a single unit and restoring PATHEXT separately in \�[0m\�[0m
...

@MotorwaySouth9 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai · 2026-01-16T01:04:56Z

Thanks @cubic-dev-ai — good catches.\n\nApplied the suggested fixes:\n- Make spy restore exception-safe via in \n- Fix PATH/Path restore on Windows (case-insensitive env keys) by restoring PATH/Path as a single unit and restoring PATHEXT separately in \n\nLocal verification:\n- bun test v1.3.6 (d530ed99)\n- bun test v1.3.6 (d530ed99)\n- \n\nCould you please re-run the review?

@MotorwaySouth9 I have started the AI code review. It will take a few minutes to complete.

cubic-dev-ai

1 issue found across 2 files (changes from recent commits).

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/tools/lsp/config.test.ts">

<violation number="1" location="src/tools/lsp/config.test.ts:33">
P2: AfterEach merges PATH/Path and always sets both, introducing a new `Path` env var on POSIX instead of restoring the original state, polluting global test environment.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

src/tools/lsp/config.test.ts

cubic-dev-ai

1 issue found across 4 files

Confidence score: 4/5

afterEach only copies one env variable back to both PATH and Path in src/tools/lsp/config.test.ts, so tests could leak a mutated PATH and introduce subtle platform-sensitive failures.
Given the limited scope and test-only impact, the risk feels manageable but worth noting before merge.
Pay close attention to src/tools/lsp/config.test.ts - ensure PATH and Path are restored independently to prevent test pollution.

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/tools/lsp/config.test.ts">

<violation number="1" location="src/tools/lsp/config.test.ts:27">
P2: afterEach does not restore PATH and Path independently, collapsing them into one value and potentially altering the original environment state between tests</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

src/tools/lsp/config.test.ts

cubic-dev-ai

1 issue found across 4 files

Confidence score: 4/5

afterEach in src/tools/lsp/config.test.ts reassigns both PATH and Path, risking environment contamination between tests and potentially hiding regressions.
Overall change carries limited runtime impact but keeping tests deterministic would improve confidence in this merge.
Pay close attention to src/tools/lsp/config.test.ts - afterEach merges PATH/Path and may leak env vars across tests.

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/tools/lsp/config.test.ts">

<violation number="1" location="src/tools/lsp/config.test.ts:27">
P2: afterEach merges PATH/Path and assigns to both, potentially creating or overwriting `Path` and leaking env state across tests</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

src/tools/lsp/config.test.ts

cubic-dev-ai

No issues found across 4 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

cubic-dev-ai

1 issue found across 4 files

Confidence score: 3/5

afterEach currently rewrites both PATH and Path even on non-Windows platforms, so test runs could leave the process with the wrong env variable casing and clobbered PATH values—this is a clear regression risk across platforms.
Risk level sits in the middle because the bug is concrete and user-facing (breaks environment state on non-Windows runs) but is limited to the test harness, so it’s tractable before merge.
Pay close attention to src/tools/lsp/config.test.ts - ensure PATH/Path are restored independently without creating new env keys.

Prompt for AI agents (all issues)


Check if these issues are valid — if so, understand the root cause of each and fix them.


<file name="src/tools/lsp/config.test.ts">

<violation number="1" location="src/tools/lsp/config.test.ts:27">
P2: afterEach does not restore PATH/Path independently, creating Path on non-Windows and potentially clobbering original env state</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}

src/tools/lsp/config.test.ts

cubic-dev-ai

No issues found across 4 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

cubic-dev-ai

No issues found across 5 files

Confidence score: 5/5

Automated review surfaced no issues in the provided summaries.
No files require special attention.

kdcokenny · 2026-01-16T12:24:02Z

Thanks for the contribution! 🚀

MotorwaySouth9 added 2 commits January 16, 2026 08:40

fix(lsp): improve Windows server detection

f9b9b59

fix(cli): avoid unix which in lsp doctor check

ca2f805